-
Notifications
You must be signed in to change notification settings - Fork 2
feat: Error when code injection fails #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Error when code injection fails #9
Conversation
3ce98d0 to
fa22533
Compare
jsumners-nr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only kind of "review" I can do for this is to rubber stamp it.
Do you agree with throwing an error in this case? An alternative would be to just return the unmodified source. |
I don't know what the ramifications are. |
bizob2828
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice change.
AbhiPrasad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wasm now throws a JavaScript error if injection fails
What's the stacktrace that gets shown?
Unfortunately it contains some wasm lines: Open to a more useful error message is |
AbhiPrasad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should try to attach some more details about the failure to the error message, like what file/function we tried to instrument.
The user supplies the library name, version and relative path to the file so it's much easier for this to be logged in JavaScript. I was saying to @bizob2828 that we should think about wrapping the wasm API with a basic JavaScript wrapper. Passing incorrect or missing parameters can cause segfaults with no stack traces. We could also use this to give more rich/helpful errors. There are potentially multiple injections/instrumentations to apply to a file and currently we only throw an error if none are applied. Now I'm thinking about it again, we should throw if any fail to apply because many times you will be instrumenting multiple methods in a class and if any fail it's bad news! |
|
I've changed it to error if any injection points fail and the message now includes the names of the functions which it failed to find: |
|
Now this is merged I'm thinking of a few use cases where we might want to apply a few different instrumentations and not care about whether they ALL succeed 🤔 |
This PR changes both the Rust and JavaScript to error if code injection fails. Previously it would inject the
diagnostics_channelrequire/import but then fail silently. If injection fails we need to know about and it we shouldn't modify the source.OrchestrionErrorhad all the unused error types removed andInjectionMatchFailureaddedInstrumentationnow tracks when code has been injected and is reset byreset_has_injectedtransformnow returns an error if no code injection points were found